SED-4539 automation package collection and accessor#615
SED-4539 automation package collection and accessor#615
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a robust framework for managing automation package YAML files programmatically, particularly focusing on plans. By providing collection-like access and precise YAML patching capabilities, it lays the groundwork for more sophisticated tooling, such as IDE integrations, that can manipulate automation package definitions while preserving their integrity and structure. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new step-automation-packages-ide module, which provides a collection-based accessor for automation packages, particularly for plans stored in YAML. This is a significant feature that allows treating file-based plans as if they were in a database collection, with support for reading, creating, updating, and deleting. The implementation cleverly uses YAML patching to preserve formatting and comments, which is a great approach. The changes are extensive and well-tested.
My review has identified a few areas for improvement. The primary concern is the exception handling strategy in several new classes, where checked exceptions are wrapped in generic RuntimeException. This can obscure error details and should be addressed. I also found a bug in YamlPlainTextPlan where an incorrect collection name is returned. Additionally, there are a couple of minor code hygiene issues that I've pointed out.
| } catch (IOException e) { | ||
| throw new RuntimeException(e); | ||
| } |
There was a problem hiding this comment.
The method is declared to throw AutomationPackageReadingException, but IOException is wrapped in a generic RuntimeException. This violates the method's contract and hides the specific nature of the error. It's better to wrap it in the declared exception type, consistent with how other IOExceptions are handled in this project.
| } catch (IOException e) { | |
| throw new RuntimeException(e); | |
| } | |
| } catch (IOException e) { | |
| throw new AutomationPackageReadingException("Failed to read automation package descriptor", e); | |
| } |
...s-yaml/src/main/java/step/automation/packages/yaml/AutomationPackageYamlFragmentManager.java
Outdated
Show resolved
Hide resolved
| @Override | ||
| public String getCollectionName() { | ||
| return "plainTextPlan"; | ||
| } |
There was a problem hiding this comment.
The collection name for plain text plans in the YAML files is plansPlainText. This method returns plainTextPlan, which is incorrect and will cause issues when trying to add or modify plain text plans in YAML fragments.
| @Override | |
| public String getCollectionName() { | |
| return "plainTextPlan"; | |
| } | |
| @Override | |
| public String getCollectionName() { | |
| return "plansPlainText"; | |
| } |
References
- Ensure consistency in naming conventions. If a constant's string representation is renamed, its corresponding object constant should also be renamed to maintain clarity and consistency across the codebase.
There was a problem hiding this comment.
This should show as outdated, not sure why it does not...
...ion-packages-manager/src/main/java/step/automation/packages/JavaAutomationPackageReader.java
Outdated
Show resolved
Hide resolved
...yaml/src/main/java/step/automation/packages/yaml/deserialization/PatchingParserDelegate.java
Outdated
Show resolved
Hide resolved
cmisev
changed the title
1 participant
No description provided.